Skip to content
This repository was archived by the owner on Feb 4, 2022. It is now read-only.

Initial work to test mongodb-core on evergreen #287

Merged
merged 39 commits into from
Mar 27, 2018
Merged

Initial work to test mongodb-core on evergreen #287

merged 39 commits into from
Mar 27, 2018

Conversation

mbroadst
Copy link
Member

@mbroadst mbroadst commented Mar 26, 2018

This includes a number of changes, but generally provides the foundation for testing our two drivers in evergreen, closing: NODE-871, NODE-873, NODE-879, NODE-880, NODE-881, NODE-883, NODE-884, NODE-1191. It also includes a few modifications to the actual tests to reduce flakiness of tests.

You can see it all builds in evergreen here: https://evergreen.mongodb.com/version/5ab7939ce3c331475bc1144a. The red boxes in the result are related to node-snappy not being built to support big endian on those ZAP systems, I have a fix for that here

mbroadst and others added 30 commits March 26, 2018 09:35
This just makes our evergreen configuration look a little more like
other drivers at very little cost to us.

NODE-884
this will pass the option down to `mongodb-download-url` and enable detection of Linux Distributions
This ditches my original custom configuration in favor of one based
on the skeleton provided in drivers-evergreen-tools.

NODE-884
These tests should either be converted to using the mock server, or
we need to update the test configuration to detect whether we are
running on evergreen or not, and use the orchestration's rest api
to do what we're asking here.

NODE-884
This is a low-impact change that allows us to potentially make far
more accurate assessments of when certain events have completed in
the pool. This also allows us to reduce the chances of flakey pool
tests
@mbroadst mbroadst requested a review from daprahamian March 26, 2018 14:34
@@ -8,7 +8,8 @@ var expect = require('chai').expect,
Bson = require('bson');

describe('Basic single server auth tests', function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is every test skiped in this describe block? We could just do describe.skip

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I'm not sure what happened here, I think these were skipped from a while ago but sure I can describe.skip it.

@@ -2,6 +2,11 @@
const ConfigurationBase = require('mongodb-test-runner').ConfigurationBase;
const f = require('util').format;

const chai = require('chai');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason we are adding these things in?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was actually glad to have found this, we often have cases where we expect(err).to.not.exist, however if it does it shows something like Expected { Object(namespace, message) } to be null which is not helpful at all. The truncation configuration option here expands to show the full message. The other options here are just the defaults, but I'm otherwise codifying that we are indeed using the defaults.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this sounds useful. Like I said above though, lets make sure it is a separate commit when we squash.

@@ -210,6 +210,7 @@ function stateTransition(self, newState) {
// Get current state
var legalStates = legalTransitions[self.state];
if (legalStates && legalStates.indexOf(newState) !== -1) {
self.emit('stateChanged', self.state, newState);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a bug that needs to be fixed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was actually using this to determine when the pool was destroyed, before I opted to use connection spies. It's a harmless addition, but adds flexibility to the design (it's a no-op if nobody is listening) so I'd opt for keeping it in.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets make sure that when we squash, this is a separate commit so we can track it.

download_and_extract "$MONGODB_DOWNLOAD_URL" "$EXTRACT"

# run tests
OS=$(uname -s | tr '[:upper:]' '[:lower:]')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel that things like this could be made into bash functions to make it clearer what is happening.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a straight rip of the existing mongo-orchestration scripts, soon to be replaced by work Sam Kleinman is doing with the curator method (read: this is going away soon).

OS=$(uname -s | tr '[:upper:]' '[:lower:]')
[ -z "$MARCH" ] && MARCH=$(uname -m | tr '[:upper:]' '[:lower:]')

if [ "$AUTH" != "noauth" ]; then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we take strings like this and stick them in variables? I'm worried about things like noauth or nossl being changed in the future.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's a bit premature, there's only a single reference to it. I'll remove the whole section of code actually, since we aren't presently running auth or ssl tests and we aren't likely to use this pattern anyway.

.evg.yml Outdated
- id: node-version
display_name: "Node.js"
values:
- id: "Argon"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we not making one of these for Carbon?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, Carbon is in the .evergreen/config.yml

.evg.yml Outdated
variables:
NODE_LTS_NAME: "boron"

buildvariants:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does evergreen not support >2D matrices? It would be nice to be able to add an axis for Mongo Version

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unfortunately not :'(

@@ -0,0 +1,629 @@
# When a task that used to pass starts to fail
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the difference between this file and .evg.yml?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack .evg.yml was accidentally commited. Removing now, and disregarding review from that file.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, then transplant all my comments from .evg.yml to evergreen/config.yml

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

honestly I already squashed a whole bunch of commits here, I'm inclined to leave it as is

@mbroadst
Copy link
Member Author

@daprahamian okay comments should be addressed

@mbroadst mbroadst merged commit 2d5dde3 into 3.0.0 Mar 27, 2018
@mbroadst mbroadst deleted the NODE-884 branch March 27, 2018 11:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants